Convert engine events to use EventEmitter2; wait for promises to resolve #237
Conversation
naveen-shetty
left a comment
naveen-shetty
left a comment
There was a problem hiding this comment.
Thanks @CacheControl for making this change. Just a couple of things I noticed.
src/engine.js
Outdated
| almanac.factValue('success-events', { event: ruleResult.event }) | ||
| return Promise.all([ | ||
| this.emitAsync('success', ruleResult.event, almanac, ruleResult), | ||
| this.emitAsync(ruleResult.event.type, ruleResult.event.params, almanac, ruleResult) |
There was a problem hiding this comment.
In the current implementation success & ruleResult.event.type is triggered sequentially. This change is triggering them concurrently. Not a problem for us but just wanted to check if it is intentional.
There was a problem hiding this comment.
another good call out! I can't see a good reason reason to deviate from the previous behavior; I've updated this to fire these in series, like it did before.
src/engine.js
Outdated
| this.emit('success', rule.event, almanac, ruleResult) | ||
| this.emit(rule.event.type, rule.event.params, almanac, ruleResult) | ||
| almanac.factValue('success-events', { event: rule.event }) | ||
| almanac.factValue('success-events', { event: ruleResult.event }) |
There was a problem hiding this comment.
As almanac.factValue returns a promise, Shouldn't this go inside Promise.all array as well?
There was a problem hiding this comment.
yikes, that promise handling has been broken for a while. good catch!
|
Thanks for the code review @naveen-shetty ! I've tweaked a few things and added docs/examples, but from a consumer's perspective the functionality hasn't changed since the 6.0.0-alpha-3 tag - feel free to use that until I release 6.0.0. I'm merging this into the |
Uses the
emitAsync()feature of EventEmitter2 to ensure promises returned to emitted events resolve before engine continues